Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

[new-rule] newline-per-chained-call #3278

Merged
merged 19 commits into from
Jan 10, 2018

Conversation

aervin
Copy link
Contributor

@aervin aervin commented Oct 4, 2017

PR checklist

  • Addresses an existing issue: #3253
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Verifies that chains containing more than n expressions be broken apart onto new lines.

Is there anything you'd like reviewers to focus on?

Configuration is still up in the air. It would be very easy to allow users to configure a "threshold" for the number of chained calls on a single line:

{
  "rules": {
    "newline-per-chained-call": [true, 2 /* the max chain length permitted on a single line */]
  }
}

Additionally, there's the question of the this keyword. Should it count as a "link" in the chain? Currently, this is ignored. Maybe an ignore-this-keyword config option would be useful?

The description/rationale may not be sufficient or what the original poster had in mind. Happy to change those.

Changelog

[new-rule] newline-per-chained-call

@@ -193,6 +193,7 @@ export const rules = {
"match-default-export-name": true,
"new-parens": true,
"newline-before-return": true,
// "newline-per-chained-call": Errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we should exclude it. We should rather find a reasonable default value.

From the "Errors" I can deduce that there are many lint errors within this project if you enable the rule. In this case you can just disable it in tslint.json

public static metadata: Lint.IRuleMetadata = {
ruleName: "newline-per-chained-call",
description: Lint.Utils.dedent`
Requires that chained property accessors be broken apart onto separate lines.`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chained property accessors or chained method calls?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also note that the corresponding eslint rule only counts method calls and not property accesses

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So method calls on their own line if the chain length--including prop accessors--is greater than n? Or should chain length include method calls only?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess method calls only is the best to start with. Otherwise you'll get a lot more errors with little benefit.
For example chai assertions:

expect({a: 1}).to.not.have.property('b');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Your single-line chai example looks fine to me, but if it had a third call I'd want each call to be on its own line:

expect({ a: 1 })
    .to.not.have
    .property('b').more.accessors // Should this be okay?
    .methodCall(1);

Copy link
Contributor Author

@aervin aervin Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this makes me realize that my scheme of counting \n chars is not very flexible... Do you have advice on how to approach this problem?

expect({ a: 1 })
    .to
    .not
    .have
    .property('b').more.methodCall(1); // Will fool the walker I think...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's best to check if there is a newline after the method call. No counting at all.

// allowing
expect({ a: 1 })
    .to.not.have.property('b');
// disallowing
expect({ a: 1 }).to.not.have
    .property('b')

The only exception is the first method call:

// not allowed
foo.bar()
    .baz();
// same with
this.bar().
    .baz();

// allowed
foo
    .bar()
    .baz();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay nice, that's a much better solution. I have only one thing I'd change in your examples, and it's purely stylistic. I think property accessors between method calls look better like this:

expect({ a: 1 })
    .to.not.have
    .property('b');

... the condition being that prop accessors between method calls cannot be on the same line as either method call. I think? It may be too convoluted for a config option. I think this is the behavior I would want in the default setting, though.

const nextAccessorOrCallExpression = (nextNode: ts.Expression): void => {
if (
isIdentifier(nextNode) ||
(isPropertyAccessExpression(nextNode) && !isThisKeyword(nextNode))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's that condition supposed to do? If nextNode is a PropertyAccessExpression it will never be ThisKeyword.


function getChainLength(node: ts.PropertyAccessExpression): number {
let chainLength = 1;
const nextAccessorOrCallExpression = (nextNode: ts.Expression): void => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could just be a while loop

public walk(sourceFile: ts.SourceFile) {
const checkForUnbrokenChain = (node: ts.Node): void => {
if (this.hasUnbrokenChain(node)) {
return this.addFailureAtNode(node, Rule.FAILURE_STRING);
Copy link
Contributor

@ajafff ajafff Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the return here is a good idea. I know it's there to avoid multiple errors on the same node, but it also ignores everything else that's nested:

// tslint:disable-next-line (I intentionally dont'want errors on this line)
just().do().stuff(
    // Unfortunately I don't get an error on the nested expression :(
    some().really().long().method().chain()
);

class NewlinePerChainedCallWalker extends Lint.AbstractWalker<void> {
public walk(sourceFile: ts.SourceFile) {
const checkForUnbrokenChain = (node: ts.Node): void => {
if (isCallExpression(node) && needsNewline(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& isPropertyAccessExpression(node.expression)

function needsNewline(node: ts.CallExpression): boolean {
const rawExpressionText = node
.getFullText()
.substr((node.expression as ts.CallExpression).expression.getFullText().length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the same with less work:

const rawExpressionText = sourceFile.text.substring((node.expression as ts.PropertyAccessExpression).expression.end, node.end);

});

this.some.nested();
~~~~~~ [ERROR]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't consider one method call a chain. Either always use a minimum of 2 or make the threshold configurable

}
}

function needsNewline(node: ts.CallExpression): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greatly simplified version, should work too:

function needsNewline(node: ts.PropertyAccessExpression, sourceFile: ts.SourceFile): boolean {
    return !isSameLine(sourceFile, node.expression.end, node.name.pos);
}

const checkForUnbrokenChain = (node: ts.Node): void => {
if (isCallExpression(node) && needsNewline(node)) {
this.addFailureAtNode(
(node.expression as ts.PropertyAccessExpression).name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to include the dot in the error's range?
That makes more sense if the dot is on the previous line:

foo.
    bar().
    baz();

Would be as simple as

this.addFailure(node.expression.name.pos - 1, node.expression.name.end, Rule.FAILURE_STRING);

}
};
ts.forEachChild(node, checkForCall);
return callExists;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ts.forEachChild is not the right choice here. The implementation would complain about this:

this.foo(bar());

Instead you can just recurse into the AST by accessing the expression property:

function hasChildCall(node: ts.PropertyAccessExpression): boolean {
    let {expression} = node;
    while (isPropertyAccessExpression(expression) || isElementAccessExpression(expression)) {
        ({expression} = expression);
    }
    return expression.kind === ts.SyntaxKind.CallExpression;
}

also note that you need to handle ElementAccessExpression for cases like this:

this.getFoo()[0].toString();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you already kinda did it this way. I was confused by the closure and the call to forEachChild.
As noted above, forEachChild is wrong here. And once you get rid of it, you don't need the callback. So you will end up with a loop like my example code.

if (isCallExpression(child) || isPropertyAccessExpression(child)) {
if (isCallExpression(child)) {
callExists = true;
return;
Copy link
Contributor

@ajafff ajafff Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you return true here, forEachChild will return early without visiting the remaining children.
you could then use return ts.forEachChild(node, checkForCall) === true below

return;
}
return checkForCall(child.expression as
| ts.CallExpression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line will not be reached for CallExpression
but the assertion is unnecessary anyway

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind, misread your code

@@ -0,0 +1,79 @@
const y: string[] = _observable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the two test cases from my comment above

and in addition:

this.foo()["bar"](); // no error here
foo().bar(); // error here? the current implementation adds one

@ajafff
Copy link
Contributor

ajafff commented Oct 19, 2017

How did we decide about the first method call? Is it allowed on the first line or not?

this.foo() // is valid with the current implementation
    .bar();

@aervin
Copy link
Contributor Author

aervin commented Oct 19, 2017

Thanks for the feedback! Concerning the first method call, I was thinking of the this keyword in particular and how it's exactly 4 characters (a common indentation length) and how often the keyword is used... Basically, I'm not sure about enforcing this

this
    .foo()
    .bar()

when this

this.foo()
    .bar()

saves a line and looks just as orderly.

@ajafff
Copy link
Contributor

ajafff commented Oct 20, 2017

I looked at the examples of the ESLint rule again. They seem to allow the first call on the same line. So I guess we can leave the current behavior unchanged.

_.chain({})
  .map(foo)
  .filter(bar);

In addition they also allow property access before the method call. Our current implementation doesn't allow this. I still don't have a strong opinion on this one. Maybe we can just add an option later if users demand it.

obj
  .prop.method()
  .method2()
  .method3().prop;

Adding a configurable threshold can also be done later.

@ChrisHuntCal
Copy link

Is there anything I can do to help move this forward? We would love to be able to implement this into our style guide!

@aervin
Copy link
Contributor Author

aervin commented Dec 9, 2017

I would too @ChrisHuntCal, but lately school and work have left little room for open source. I have no problem with someone continuing this (or taking a fresh crack at it). I do remember there being a PR for an "indentation" helper for tslint. If that was merged in, could this rule have a fixer?

@ChrisHuntCal
Copy link

Cool! I'll try to get acquainted with the code and see where I get.

@aervin
Copy link
Contributor Author

aervin commented Dec 13, 2017

Sounds good @ChrisHuntCal. I think the most pressing concern is addressing this case: this.someMethod()["elementAccessExpression"](). The rule should throw at the element access expression but currently ignores them.

@ajafff
Copy link
Contributor

ajafff commented Dec 13, 2017

@aervin this.someMethod()["elementAccessExpression"]() should not be an error.
Inserting a line break before the element access can cause a lot of confusion, because it looks like an array.

I just meant you shouldn't stop recursing on an ElementAccessExpression when searching for method calls.

@aervin
Copy link
Contributor Author

aervin commented Dec 13, 2017

@ajafff this may be okay then? I added the following test cases:

this.getFoo()[0].toString(); // errors here

foo().bar(); // errors here

this.foo()["bar"](); // no problems with this

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with comment addressed

package.json Outdated
@@ -81,4 +81,4 @@
"engines": {
"node": ">=4.8.0"
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert this change

@aervin
Copy link
Contributor Author

aervin commented Dec 14, 2017

@ajafff package.json fixed. CI error is a timeout issue.

@JarvisPrestidge
Copy link

@aervin @ajafff Any movement on this would be greatly appreciated 🙂

@adidahiya adidahiya merged commit 4117f16 into palantir:master Jan 10, 2018
HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
@et
Copy link

et commented Aug 16, 2018

Hey hopefully I can ask a quick question about this.
Is it possible to enforce the indentation on the newline?

e.g. both of these pass

  private isValidStageName(name: string): boolean {
    return Object.values(PipelineStages)
    .includes(name);
  }
  private isValidStageName(name: string): boolean {
    return Object.values(PipelineStages)
      .includes(name);
  }

but I would only like the second to pass.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants